Support removing networking from stopped VM #661
Conversation
|
Thanks for working on this bug. This could fix the VM restart issue due to the same duplicate IP allocation error as seen in #504 (comment) . |
There was a problem hiding this comment.
Hi @innobead 👋
Thanks for digging into this 👍
I have just some nits, and @darkowlzz has some points that would be good to fix before merging. Otherwise it's looking very good!
|
@darkowlzz @twelho thanks for the review 👍 All comments/suggestions resolved. |
|
Hi @innobead, thanks for your contributions! I have one more improvement to get rid of the diff --git a/pkg/network/cni/cni.go b/pkg/network/cni/cni.go
index 7f953da1..c3e6877d 100644
--- a/pkg/network/cni/cni.go
+++ b/pkg/network/cni/cni.go
@@ -189,7 +189,7 @@ func cniToIgniteResult(r *gocni.CNIResult) *network.Result {
return result
}
-func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string, isRunning bool) (err error) {
+func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string) (err error) {
if err = plugin.initialize(); err != nil {
return err
}
@@ -208,13 +208,10 @@ func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string, isRun
return nil
}
- netnsPath := ""
- if isRunning {
- netnsPath = fmt.Sprintf(netNSPathFmt, c.PID)
- if c.PID == 0 {
- log.Info("CNI failed to retrieve network namespace path, PID was 0")
- return nil
- }
+ netnsPath := fmt.Sprintf(netNSPathFmt, c.PID)
+ if c.PID == 0 {
+ netnsPath = "" // Using a blank netns triggers a best effort cleanup
+ log.Warn("CNI failed to retrieve network namespace path, PID was 0")
}
return plugin.cni.Remove(context.Background(), containerID, netnsPath)
diff --git a/pkg/network/docker/docker.go b/pkg/network/docker/docker.go
index e3377baa..4d419267 100644
--- a/pkg/network/docker/docker.go
+++ b/pkg/network/docker/docker.go
@@ -44,7 +44,7 @@ func (plugin *dockerNetworkPlugin) SetupContainerNetwork(containerID string, _ .
}, nil
}
-func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string, _ bool) error {
+func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string) error {
// no-op for docker, this is handled automatically
return nil
}
diff --git a/pkg/network/types.go b/pkg/network/types.go
index 89273292..f91ce053 100644
--- a/pkg/network/types.go
+++ b/pkg/network/types.go
@@ -21,7 +21,7 @@ type Plugin interface {
SetupContainerNetwork(containerID string, portmappings ...meta.PortMapping) (*Result, error)
// RemoveContainerNetwork is the method called before a container using the network plugin can be deleted
- RemoveContainerNetwork(containerID string, isRunning bool) error
+ RemoveContainerNetwork(containerID string) error
}
type Result struct {
diff --git a/pkg/operations/remove.go b/pkg/operations/remove.go
index 1fbf907b..2786c254 100644
--- a/pkg/operations/remove.go
+++ b/pkg/operations/remove.go
@@ -83,7 +83,7 @@ func StopVM(vm *api.VM, kill, silent bool) error {
}
// Remove VM networking
- if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID()), true); err != nil {
+ if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID())); err != nil {
log.Warnf("Failed to cleanup networking for stopped container %s %q: %v", vm.GetKind(), vm.GetUID(), err)
return err
@@ -116,7 +116,7 @@ func StopVM(vm *api.VM, kill, silent bool) error {
return nil
}
-func removeNetworking(containerID string, isRunning bool) error {
+func removeNetworking(containerID string) error {
log.Infof("Removing the container with ID %q from the %q network", containerID, providers.NetworkPlugin.Name())
- return providers.NetworkPlugin.RemoveContainerNetwork(containerID, isRunning)
+ return providers.NetworkPlugin.RemoveContainerNetwork(containerID)
} |
|
@twelho I can clean up a bit by using this patch but I doubt if this will cause some side effect. Because ignite/vendor/github.com/containerd/go-cni/cni.go Lines 172 to 182 in 0a18927 |
|
Upon stopping a VM the container running Since you already pass I tested this locally using Docker: $ sudo ignite --runtime docker start a
INFO[0001] Networking is handled by "cni"
INFO[0001] Started Firecracker VM "a6c4217ec5f07295" in a container with ID "22a0997d7119e75c17ed3ff6b9a34f46eba82d1bdebe29e3035b6191091c6939"
$ sudo docker ps -a
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
22a0997d7119 weaveworks/ignite:dev "/usr/local/bin/igni…" 2 seconds ago Up 2 seconds ignite-a6c4217ec5f07295
$ sudo docker stop ignite-a6c4217ec5f07295 # Stopping will automatically remove the container using Docker's autoremove
ignite-a6c4217ec5f07295
$ sudo ignite --runtime docker stop a
WARN[0000] VM "a6c4217ec5f07295" is not running but trying to cleanup networking for stopped container
INFO[0000] Removing the container with ID "ignite-a6c4217ec5f07295" from the "cni" network
INFO[0000] CNI failed to retrieve network namespace path: Error: No such container: ignite-a6c4217ec5f07295
$and using containerd: $ sudo ignite start b
INFO[0000] Networking is handled by "cni"
INFO[0000] Started Firecracker VM "b7ddccf23b066051" in a container with ID "ignite-b7ddccf23b066051"
$ sudo ctr -n firecracker task ls
TASK PID STATUS
ignite-b7ddccf23b066051 126854 RUNNING
$ sudo ctr -n firecracker task kill ignite-b7ddccf23b066051
$ sudo ctr -n firecracker task ls
TASK PID STATUS
ignite-b7ddccf23b066051 126854 STOPPED
$ sudo ctr -n firecracker task rm ignite-b7ddccf23b066051
$ sudo ignite stop b
WARN[0000] VM "b7ddccf23b066051" is not running but trying to cleanup networking for stopped container
INFO[0000] Removing the container with ID "ignite-b7ddccf23b066051" from the "cni" network
INFO[0000] CNI failed to retrieve network namespace path: no running task found: task ignite-b7ddccf23b066051 not found: not found
$Both return an exit code of 0, this is intended. |
|
Thanks for the testing 👍, the patch applied. |
|
LGTM 👍 |
Fix #660